Skip to content

feat: add Helmet security headers and restrict CORS to known origin#116

Open
GitAddRemote wants to merge 5 commits intomainfrom
fix/ISSUE-94-helmet-cors
Open

feat: add Helmet security headers and restrict CORS to known origin#116
GitAddRemote wants to merge 5 commits intomainfrom
fix/ISSUE-94-helmet-cors

Conversation

@GitAddRemote
Copy link
Copy Markdown
Owner

Summary

  • Installs helmet package for comprehensive HTTP security headers (CSP, HSTS, X-Frame-Options, etc.)
  • Applies app.use(helmet()) as the first middleware in the bootstrap pipeline, before all other middleware
  • Replaces app.enableCors() wildcard with origin-restricted CORS using ALLOWED_ORIGIN env variable
  • Retrieves config via ConfigService (consistent with rest of app) instead of raw process.env
  • Added ALLOWED_ORIGIN=http://localhost:5173 to .env.example with production comment (https://station.drdnt.org)

Test plan

  • Run pnpm install to install the helmet package
  • Start backend with pnpm dev:backend
  • Check response headers — should include X-Frame-Options, Strict-Transport-Security, X-Content-Type-Options, etc.
  • Make a cross-origin request from http://localhost:5173 — should succeed
  • Make a request from a different origin — should be rejected with CORS error
  • Set ALLOWED_ORIGIN=https://example.com and verify only that origin is allowed

Closes #94

Copilot AI review requested due to automatic review settings April 12, 2026 00:02
Install helmet for security headers, apply before all other middleware.
Restrict CORS from wildcard to configurable ALLOWED_ORIGIN env variable.
CORS allows credentials and standard HTTP methods.

Production CORS origin should be set to https://station.drdnt.org.
Development default: http://localhost:5173 (see .env.example).

Closes #94
Lockfile was out of sync with the helmet, cookie-parser, and
@types/cookie-parser additions, causing CI frozen-lockfile check to fail.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the NestJS backend by adding standard HTTP security headers via Helmet and replacing wildcard CORS with an origin-restricted configuration driven by an ALLOWED_ORIGIN environment variable.

Changes:

  • Added helmet and registered it early in the bootstrap pipeline.
  • Replaced app.enableCors() defaults with a restricted CORS config using ALLOWED_ORIGIN and credentials: true.
  • Documented ALLOWED_ORIGIN in backend/.env.example (and updated dependency lockfile).

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

File Description
pnpm-lock.yaml Adds lock entries for Helmet (and other newly-added deps).
backend/src/main.ts Registers Helmet and config-driven, credentialed CORS.
backend/package.json Adds Helmet and additional dependencies/types.
backend/.env.example Documents ALLOWED_ORIGIN for local/prod usage.
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@GitAddRemote GitAddRemote force-pushed the fix/ISSUE-94-helmet-cors branch from 1dc5256 to 835d733 Compare April 12, 2026 00:06
@GitAddRemote GitAddRemote self-assigned this Apr 12, 2026
- Configure Helmet with explicit CSP directives that allow Swagger UI's
  inline scripts and styles (unsafe-inline on scriptSrc/styleSrc);
  default Helmet CSP blocks Swagger and leaves the docs page blank
- Extract ALLOWED_ORIGIN into a variable with a localhost fallback so
  CORS never silently falls back to permissive defaults when the env var
  is unset; origin validation is enforced at startup in PR #118
- Remove cookie-parser, @types/cookie-parser, and joi from package.json
  as they are unused on this branch; those deps are introduced in the
  cookie auth (#117) and env validation (#118) PRs respectively
Copilot AI review requested due to automatic review settings April 13, 2026 05:08
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Use strict CSP (no 'unsafe-inline') in production; Swagger is
  disabled in production so the relaxed CSP it requires is unnecessary
  and should not weaken security there
- Throw at startup in production if ALLOWED_ORIGIN is unset; dev/test
  environments still fall back to http://localhost:5173
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +18 to 22
const configService = app.get(ConfigService);
const port = configService.get<string>('PORT') || 3001;
const appName = configService.get<string>('APP_NAME') || 'STATION BACKEND';
const isProduction = process.env.NODE_ENV === 'production';

Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description says config is retrieved via ConfigService instead of raw process.env, but isProduction (and the Swagger gating below) still read process.env.NODE_ENV directly. Consider reading NODE_ENV via ConfigService (or centralizing the env access) and reusing isProduction for the later production checks to keep configuration consistent and avoid divergence.

Copilot uses AI. Check for mistakes.
Comment on lines +28 to +33
app.use(
helmet({
contentSecurityPolicy: {
directives: {
defaultSrc: [`'self'`],
baseUri: [`'self'`],
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To meet the issue’s acceptance criteria (and to avoid relying on Helmet defaults), explicitly configure the headers that must have specific values in all responses (e.g., set frameguard to deny for X-Frame-Options: DENY, and configure hsts for production so Strict-Transport-Security is guaranteed).

Copilot uses AI. Check for mistakes.
Comment on lines +45 to +52
const allowedOrigin = configService.get<string>('ALLOWED_ORIGIN')?.trim();
if (!allowedOrigin && isProduction) {
throw new Error('Missing required environment variable: ALLOWED_ORIGIN');
}
app.enableCors({
origin: allowedOrigin ?? 'http://localhost:5173',
credentials: true,
methods: ['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS'],
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After .trim(), allowedOrigin can be an empty string (e.g., ALLOWED_ORIGIN=" "). In non-production this won’t throw, and origin: allowedOrigin ?? 'http://localhost:5173' will pass '' to the CORS config (since ?? doesn’t treat empty string as nullish), which can effectively break CORS in dev. Consider normalizing empty-string to undefined (or use || after trimming) before applying the fallback.

Copilot uses AI. Check for mistakes.
- Derive isProduction from configService.get('NODE_ENV') so all env
  access goes through the validated ConfigModule rather than mixing
  process.env reads with ConfigService calls
- Reuse isProduction for all three conditional checks (CSP, Swagger,
  startup log) — no more raw process.env.NODE_ENV in bootstrap
- Add explicit frameguard: deny (X-Frame-Options: DENY) and hsts with
  1-year max-age + includeSubDomains in production (disabled in dev)
- Normalize empty-string ALLOWED_ORIGIN to undefined via || so a
  whitespace-only value gets the same fail-fast treatment as unset
- Use get<number>('PORT') and ?? so port is always a number
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tech Story: Add Helmet and restrict CORS to known origin

2 participants